-
Notifications
You must be signed in to change notification settings - Fork 19
CoRIM SFR Profile and Example #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
thomas-fossati
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have checked the extension CDDL and the associated example: LGTM!
A couple of suggestions:
-
Having both checked for correctness automatically by the CI would make it more robust in the face of change, especially if the change is accidental.
-
The CoRIM CDDL is released as an artefact of the CoRIM pipeline at each version drop (e.g.: https://github.com/ietf-rats-wg/draft-ietf-rats-corim/releases/tag/cddl-draft-ietf-rats-corim-08). While it’s under development, you might want to simplify tracking the alignment somewhat automatically (see for example what we do in CoSERV: https://github.com/rats-endorsements-distribution/draft-howard-rats-coserv/tree/main/cddl/comid.mk)
BTW, it’s great to see CoRIM being used more widely 👍
Documentation/corim_profile/examples/ocp-safe-sfr-fw-example.diag
Outdated
Show resolved
Hide resolved
Documentation/corim_profile/examples/ocp-safe-sfr-fw-example.diag
Outdated
Show resolved
Hide resolved
thomas-fossati
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
I left a couple of (FYI) comments. Feel free to ignore them.
Thanks for lookin this over @thomas-fossati I have addressed your two points by adding a simple workflow, once the repo admins enable GitHub Actions with some Runners...we should be able to test it out. In the meantime could you take a quick look and sanity check the workflow? |
|
This commit squashes all the intermediate commits from PR opencomputeproject#49 Signed-off-by: Alex Tzonkov <[email protected]>
Signed-off-by: Alex Tzonkov <[email protected]>
Signed-off-by: Alex Tzonkov <[email protected]>
Signed-off-by: Alex Tzonkov <[email protected]>
|
@ericeilertson @nickhummel from my side this is ready to get squashed and merged. If we need additional work on the CoRIM profile specification we can open a new PR for that. Thank you all for the help getting this done! |
rob-tetrel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
|
Great stuff! It’d be super if someone could provide the SFR serialisation/deserialisation code for veraison/corim (and/or veraison/corim-rs). Then, one could use the stock |
- Enhancing the human readable script to handle signed CBOR and display all fields in human readable form. - Addressing fix from Rob Signed-off-by: Alex Tzonkov <[email protected]>
Signed-off-by: Alex Tzonkov <[email protected]>
Signed-off-by: Alex Tzonkov <[email protected]>
Signed-off-by: Alex Tzonkov <[email protected]>
Documentation/corim_profile/OCP-SAFE-CoRIM-Extension-Profile-Specification.md
Show resolved
Hide resolved
|
|
||
| ### Firmware Identifiers | ||
|
|
||
| Firmware identifiers provide detailed information about the firmware components that were subject to security review: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't ocp-safe-sfr-map linked to a specific comid component? I'm not understanding what purpose fw-identifiers serves in the ocp-safe-sfr-map, as this information should be available (or inferable) from the component the ocp-safe-sfr-map is attached to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when this came up in the meeting it was agreed to keep these as optional attributes that can be filled out by the SRP to help with standalone use-cases beyond attestation. These are some of the ones mentioned in the meeting:
- Being able to know the on-disk hash of the FW binary file an SFR endorses during platform provisioning
- In the case of the SRC only audits the src-manifest provides the ability to describe all source files audited
- Repo tag helps with logistics of future/delta audits by providing a starting point for changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm asking a slightly different question here:
- Won't this endorsement already be associated with an exact hash of a specific FW component?
- If so, what information does the fw-identifier provide that cannot be inferred from that hash? (which will necessarily correspond to a specific commit in a specific repo)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the intent is to make it easier for a human to understand what specific Firmware is being endorsed by looking at the fw-identifier if it is present. The 'exact hash' you mention is the runtime hash of the FW which is probably only in one other location, the Vendor CoRIM for that Firmware release.
Further for source only audits there is no 'runtime hash' that an SRP can endorse. Hence a repo tag is probably the best way to identify the source for the audit, and can be verified using the manifest hash.
Similarly, if there is no runtime measurement being used for Firmware that doesn't get measured for attestation, an On-disk hash gives the recipient of such FW the ability to verify that the binary file they received indeed corresponds to what the SRP audited.
Documentation/corim_profile/OCP-SAFE-CoRIM-Extension-Profile-Specification.md
Show resolved
Hide resolved
| Security issues identified during the review are represented using the `issue-entry` structure: | ||
|
|
||
| ```cddl | ||
| issue-entry = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the CVSS-related entries should be generic to allow for other vuln rating systems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we talked about making the assessment-schema a required field that can be an cvss-schema or jil-schema, etc. Something along the lines of:
issue-entry = {
&(title: 0) => tstr
&(description: 1) => tstr
&(assessment-scheme: 2) => $assessment-scheme
?&(cwe: 3) => tstr
?&(cve: 4) => tstr
* $$ocp-safe-issue-entry-ext
}
$assessment-scheme /= cvss-scheme
cvss-scheme = {
&(cvss-score: 0) => tstr
&(cvss-vector: 1) => tstr
&(cvss-version: 2) => tstr
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the naming for assessment-scheme given that block is capturing the issue scoring as well. Maybe "issue-assessment" instead? Otherwise, I like the arrangement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about something like:
issue-entry = {
&(title: 0) => tstr
&(description: 1) => tstr
&(assessment: 2) => $assessment
?&(cwe: 3) => tstr
?&(cve: 4) => tstr
* $$ocp-safe-issue-entry-ext
}
$assessment /= cvss
cvss = {
&(score: 0) => tstr
&(vector: 1) => tstr
&(version: 2) => tstr
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me. I'm not familiar enough with JIL to comment on the proposed scheme below but I'll prod some folks who are for feedback.
Documentation/corim_profile/OCP-SAFE-CoRIM-Extension-Profile-Specification.md
Show resolved
Hide resolved
Documentation/corim_profile/OCP-SAFE-CoRIM-Extension-Profile-Specification.md
Show resolved
Hide resolved
|
|
||
| The specification supports the following device categories: | ||
|
|
||
| **Table 3: Device Category Mappings** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a category for security chips (eROT/PaROT/dTPM/etc). Maybe also a category for FPGA/CPLD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
historically this field has been treated as a free-form text description. Does limiting to a numerical enumeration provide the granularity needed? who actually consumes this value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From CBOR perspective "free form text" is discouraged. We did discuss this in one of the prior meetings and agreed to enumerate them. We can extend the categories as needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is my proposal:
| Category | Value | Description |
|---|---|---|
| storage | 0 | Storage devices (SSDs, HDDs, etc.) |
| network | 1 | Network interface controllers and switches |
| cpu | 2 | General Processor (microprocessors, microcontrollers, etc.) |
| specialized_processor | 3 | Specialized Processors (GPU, APU, NPU, TPU, DPU, VPU, QPU, etc.) |
| mc | 4 | Management controllers (BMC, SMC, EC, etc.) |
| root_of_trust | 5 | Root of Trust (iROT, PROT, EROT, etc.) |
| programable_logic | 6 | Programable Logic (FPGA, CPLD, etc.) |
| source_only | 7 | Reusable Source Code (e.g. libSPDM, OpenSSL, etc.) |
Does address your concern @syncsrc-nv ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is good. A couple comments on the proposed change:
- Should "iROT" be an example for the root-of-trust option? It's not a stand-alone device, I would expect the category for an iROT to be that of device the iROT is embedded in.
- May also want to expand the ROT option to include other stand-alone secure-element and TPM type devices that might not normally be classified as a ROT due to their limited functionality. Or provide another category for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another possible category suggested to me: power-delivery (for UPS/PSU/PDU/etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to better understand the use case for the device category field. From my perspective, each SAFE entry is already uniquely identified by the tuple (Vendor, Model, ClassId), and it's the device vendor's responsibility to document what each tuple represents in their reference values.
My concern with adding an enumerated category field is that it may blur the line between the endorser's role (augmenting device evidence) and the vendor's role (documenting their products). Additionally, any fixed enumeration risks becoming outdated as new device types emerge that don't fit existing categories. @ericeilertson
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My interpretation was that this field was describing the evaluation rubric that was applied, which would not come from the device vendor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, this was historically descriptive free-form text provided by the vendor. I think Fabrizio is hinting that maybe this field can be removed entirely. It is definitely of unclear value in its current form, and has noted issues in the proposed form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericeilertson I tend to agree that the value of the 'category' field is not clear. I would be in favor of simplifying the schema and removing it all together. Can we bring this as an open in the meeting, or even an email discussion to try and expedite the discussion/decision?
Documentation/corim_profile/OCP-SAFE-CoRIM-Extension-Profile-Specification.md
Show resolved
Hide resolved
Documentation/corim_profile/OCP-SAFE-CoRIM-Extension-Profile-Specification.md
Show resolved
Hide resolved
Documentation/corim_profile/OCP-SAFE-CoRIM-Extension-Profile-Specification.md
Show resolved
Hide resolved
|
Here is one possible schema for JIL, can we get some JIL experts input? |
No description provided.